[2.5] Re-allow .install files containing '..' - #7009
Conversation
rjbou
left a comment
rjbou
left a comment
There was a problem hiding this comment.
There is a missing test that checks escapability (for ex { "tmp/../../../../middle/dosiero" }. That's how a found #7010.
Besides this real_path behaviour, I understand the reason to reintroduce .. handling in 2.5.2, but i don't think we should backport it on master, at least for the moment. #6938 is open to propose an alternative solution, but we can discuss it later (but definitively before 2.6 release)
kit-ty-kate
added
the
PR: QUEUED
bc9c8f2 to
8c0c077
Compare
| share: [ | ||
| "a-file" { "../../a-file" } | ||
| ] | ||
| misc: [ |
There was a problem hiding this comment.
I don't know if it worth testing misc for this test, it needs to be an absolute path and we are testing relative paths
There was a problem hiding this comment.
since one wrong thing in a field invalidates the whole field i think it's worth checking since this pattern isn't checked
| ## Doc | ||
|
|
||
| ## Security fixes | ||
| * Invalidate .install fields containing absolute destination filepath except when in the `misc` field [#6897 #7008 @kit-ty-kate] |
There was a problem hiding this comment.
| * Invalidate .install fields containing absolute destination filepath except when in the `misc` field [#6897 #7008 @kit-ty-kate] | |
| * Re-allow `..` in `.install` files, partially reverts 2.5.1 #6879 [#7009 @kit-ty-kate] |
There was a problem hiding this comment.
it doesn't really matter, it's master_changes which is just for us anyway and we're doing a release right after this.
8c0c077 to
26eb749
Compare
26eb749 to
97d1076
Compare
rjbou
left a comment
rjbou
left a comment
There was a problem hiding this comment.
lgtm with green CI
On the 2nd commit message, I think it is better to avoid having links, especially non permanent ones on commit messages. If the message is deleted, or the zulip is no longer online, it is not possible to find out the origin that you wanted to highlight.
…tination filepath trying to escape their scope
…sary intermediate directories for paths that contain '..'
97d1076 to
b3d1df8
Compare
kit-ty-kate
removed
the
PR: QUEUED
|
Ignoring the unfinished Solvers-macOS jobs whose result would be unrelated to this PR anyway, and the failing cygwin build (#7007) |
Not sure if this is the place to discuss it, but the easiest path forward is to include this behavior and the new root/rootexec in a release, since dune wants the new fields released before targetting them. The release following that could restrict |
That's what i'm prone to, with some logging in case of '..'. I think we'll discuss it on monday dev meeting to settle it. |
3 participants
Backport of #7008 on 2.5
Queued on #7013